some reserve-mb adjusts #35
Conversation
iasunsea
force-pushed
the
master
branch
3 times, most recently
from
5a8f74f
to
c68d04f
Compare
|
@coiby um, ping? |
|
@VladimirSlavik Thanks for the reminder! @iasunsea Thanks for the PR! Unfortunately, it fails some tests, $ make runpylint
***Running pylint checks***
pylint com_redhat_kdump -E 2> /dev/null
************* Module com_redhat_kdump.gui.spokes.kdump
com_redhat_kdump/gui/spokes/kdump.py:94:8: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:95:8: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:110:16: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:115:8: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:147:49: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
com_redhat_kdump/gui/spokes/kdump.py:218:32: E1101: Instance of 'KdumpSpoke' has no '_toBeReservedSpin' member (no-member)
make: *** [Makefile:93: runpylint] Error 2
$ make unittest
***Running unittests checks***
pytest -vv test/unit_tests
==================================================================================== test session starts ====================================================================================
platform linux -- Python 3.11.3, pytest-7.1.3, pluggy-1.0.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /home/coiby/kexec-tools/kdump-anaconda-addon
collected 29 items
test/unit_tests/test_common.py::KdumpCommonTestCase::test_memory_bound_aarch64 PASSED [ 3%]
test/unit_tests/test_common.py::KdumpCommonTestCase::test_memory_bound_ppc64 PASSED [ 6%]
test/unit_tests/test_common.py::KdumpCommonTestCase::test_memory_bound_x86 PASSED [ 10%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_fadump_enabled PASSED [ 13%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_get_default_crashkernel PASSED [ 17%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_get_default_crashkernel_empty PASSED [ 20%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_get_default_crashkernel_file_not_found PASSED [ 24%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_crashkernel_auto PASSED [ 27%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_crashkernel_auto_fallback PASSED [ 31%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_crashkernel_auto_fallback_legacy PASSED [ 34%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_disabled PASSED [ 37%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_configuration_kdump_enabled PASSED [ 41%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_installation_kdump_disabled PASSED [ 44%]
test/unit_tests/test_installation.py::KdumpInstallationTestCase::test_installation_kdump_enabled PASSED [ 48%]
test/unit_tests/test_interface.py::KdumpInterfaceTestCase::test_fadump_enabled PASSED [ 51%]
test/unit_tests/test_interface.py::KdumpInterfaceTestCase::test_kdump_enabled PASSED [ 55%]
test/unit_tests/test_interface.py::KdumpInterfaceTestCase::test_reserved_memory FAILED [ 58%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_default PASSED [ 62%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_disable PASSED [ 65%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_enable PASSED [ 68%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_enablefadump PASSED [ 72%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_auto PASSED [ 75%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_aarch64_with_low FAILED [ 79%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_aarch64_with_normal FAILED [ 82%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_aarch64_with_up FAILED [ 86%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_invalid PASSED [ 89%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_x86_with_low FAILED [ 93%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_x86_with_normal FAILED [ 96%]
test/unit_tests/test_kickstart.py::KdumpKickstartTestCase::test_ks_reserve_mb_x86_with_up FAILED [100%]
========================================================================================= FAILURES ==========================================================================================
________________________________________________________________________ KdumpInterfaceTestCase.test_reserved_memory ________________________________________________________________________
self = <test.unit_tests.test_interface.KdumpInterfaceTestCase testMethod=test_reserved_memory>
def test_reserved_memory(self):
self._interface.ReservedMemory = "256"
> self._check_properties_changed("ReservedMemory", "256")
test/unit_tests/test_interface.py:53:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test/unit_tests/test_interface.py:35: in _check_properties_changed
self._callback.assert_called_once_with(
/usr/lib64/python3.11/unittest/mock.py:945: in assert_called_once_with
return self.assert_called_with(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <PropertiesChangedCallback id='140662632707024'>, args = ('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256'}, []), kwargs = {}
expected = call('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256'}, []), actual = call('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256M'}, [])
_error_message = <function NonCallableMock.assert_called_with.<locals>._error_message at 0x7fee92acd4e0>, cause = None
def assert_called_with(self, /, *args, **kwargs):
"""assert that the last call was made with the specified arguments.
Raises an AssertionError if the args and keyword args passed in are
different to the last call to the mock."""
if self.call_args is None:
expected = self._format_mock_call_signature(args, kwargs)
actual = 'not called.'
error_message = ('expected call not found.\nExpected: %s\nActual: %s'
% (expected, actual))
raise AssertionError(error_message)
def _error_message():
msg = self._format_mock_failure_message(args, kwargs)
return msg
expected = self._call_matcher(_Call((args, kwargs), two=True))
actual = self._call_matcher(self.call_args)
if actual != expected:
cause = expected if isinstance(expected, Exception) else None
> raise AssertionError(_error_message()) from cause
E AssertionError: expected call not found.
E Expected: mock('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256'}, [])
E Actual: mock('org.fedoraproject.Anaconda.Addons.Kdump', {'ReservedMemory': '256M'}, [])
... |
iasunsea
force-pushed
the
master
branch
2 times, most recently
from
95b07c2
to
e996214
Compare
|
@coiby i have amend the code, please take a C C |
There was a problem hiding this comment.
reserved_memory is architecture-independent so there is no need to create fixtures for each architecture. You shall be able to simplify the tests.
There was a problem hiding this comment.
https://github.com/rhinstaller/kdump-anaconda-addon/blob/master/com_redhat_kdump/common.py#L71-L82, it make diffrent architecture lowerBound , so to test diffrent architecture is need.
There was a problem hiding this comment.
Thanks for correcting me! But I think my point still stands. Although different architectures have different lowerBound and minUsable values, you only need to test one arch if choose a proper testing strategy. If you partition ReservedMemory by a) ReservedMemory <= lowerBound b) b) lowerBound < ReservedMemory < upperBound ReservedMemory >= upperBound, you only need to mock one arch.
test/unit_tests/test_kickstart.py
Outdated
There was a problem hiding this comment.
I don't see no any need to create fixtures for each architecture here either. And if reserved_memory has already been thoroughly tested, maybe one or two test cases are sufficient?
There was a problem hiding this comment.
ok, i will simplify the tests of test_kickstart.py
iasunsea
force-pushed
the
master
branch
2 times, most recently
from
6fd6147
to
2edacf4
Compare
|
@coiby i have amend the code, please again. |
|
i have saw the failed， i will change it. |
|
pls do the check |
|
@iasunsea One unit test still fails. Btw, I'll change the setting so you have the permission to trigger the workflow. Or you can run the tests locally by |
ok， i think |
|
it‘s ok, pls check |
test/unit_tests/test_interface.py
Outdated
| def test_reserved_memory(self): | ||
| @patch("builtins.open", MockBuiltinRead(AARCH64_NFO_FIXTURE)) | ||
| @patch("blivet.arch.get_arch", return_value="aarch64") | ||
| def test_reserved_memory_aarch64(self, _mock_read): |
There was a problem hiding this comment.
I don't think you should change this test or test/unit_tests/test_interface.py at all. All tests here in test/unit_tests/test_interface.py are to make sure PropertiesChanged signal will be emitted and then the corresponding callback functions will catch it.
There was a problem hiding this comment.
Because different plat or different machine with test the “_lower“ ”_upper” will have different value. And when we just want to make sure PropertiesChanged will be emitted, we just control "check_reserved_memory" return value.
com_redhat_kdump/service/kdump.py
Outdated
| if int(value) > self._upper: | ||
| value = str(int(self._upper)) | ||
| if int(value) < self._lower: | ||
| value = str(int(self._lower)) |
There was a problem hiding this comment.
Maybe you can put the above code inside a standalone function and unit-test this function (choose three values, one < lowerBound, one between lowerBound and upperBound and one > upperBound) instead? I think this shall make the test easier and there is also no need to change test/unit_tests/test_kickstart.py.
There was a problem hiding this comment.
different plat or different machine with test the “_lower“ ”_upper” will have different value. the set value oftest/unit_tests/test_kickstart.py must between the “_lower“ ”_upper” ， this is the pr we want to do. And i have change to control "getMemoryBounds" to give numeric value.
|
@coiby it‘s ok, pls. |
| @@ -48,7 +50,15 @@ def test_fadump_enabled(self): | |||
| self._check_properties_changed("FadumpEnabled", True) | |||
| self.assertEqual(self._interface.FadumpEnabled, True) | |||
|
|
|||
| def test_reserved_memory(self): | |||
| @patch("com_redhat_kdump.service.kdump.KdumpService.check_reserved_memory", return_value="256") | |||
| def test_reserved_memory(self, _mock_read): | |||
There was a problem hiding this comment.
It seems there is no need to change test_reserved_memory, or am I mistaken?
| self._service._lower , self._service._upper, self._service._step = common.getMemoryBounds() | ||
| self.assertEqual(self._service.check_reserved_memory("900"), "800") | ||
| self.assertEqual(self._service.check_reserved_memory("400"), "500") | ||
| self.assertEqual(self._service.check_reserved_memory("600"), "600") |
There was a problem hiding this comment.
For this test, you should patch com_redhat_kdump.service.kdump.getMemoryBounds instead because you should patch a function where it's used (imported) instead of where it's defined. Though unittest doesn't support parameterized tests, you can still simplify the code a bit,
@patch("com_redhat_kdump.service.kdump.getMemoryBounds", return_value = (500, 800, 1))
def test_check_reserved_memory(self, mocker):
service1 = KdumpService()
for memory, reserved in [("900", "800"), ("400", "500"), ("600", "600")]:
self.assertEqual(service1.check_reserved_memory(memory), reserved)| from unittest.mock import Mock | ||
|
|
||
| from com_redhat_kdump import common | ||
| from .mock import MockBuiltinRead |
There was a problem hiding this comment.
MockBuiltinRead is no longer used.
| @@ -68,7 +70,10 @@ def test_ks_disable(self): | |||
| %end | |||
| """) | |||
|
|
|||
| def test_ks_reserve_mb(self): | |||
| @patch("com_redhat_kdump.common.getMemoryBounds", return_value=(160, 800, 1)) | |||
| def test_ks_reserve_mb(self, _mock_read): | |||
There was a problem hiding this comment.
For this test, I would suggest to patch for setup instead,
diff --git a/test/unit_tests/test_kickstart.py b/test/unit_tests/test_kickstart.py
index 4f063de..66817ac 100644
--- a/test/unit_tests/test_kickstart.py
+++ b/test/unit_tests/test_kickstart.py
@@ -2,13 +2,13 @@ from textwrap import dedent
from unittest.case import TestCase
from unittest.mock import patch
from com_redhat_kdump import common
-from .mock import MockBuiltinRead
from com_redhat_kdump.service.kdump import KdumpService
class KdumpKickstartTestCase(TestCase):
- def setUp(self):
+ @patch("com_redhat_kdump.service.kdump.getMemoryBounds", return_value=(160, 800, 1))
+ def setUp(self, mocker):
# Show unlimited diff.
self.maxDiff = None
@@ -70,10 +70,7 @@ class KdumpKickstartTestCase(TestCase):
%end
""")
- @patch("com_redhat_kdump.common.getMemoryBounds", return_value=(160, 800, 1))
- def test_ks_reserve_mb(self, _mock_read):
- self._service._lower , self._service._upper, self._service._step = common.getMemoryBounds()
- self.assertEqual((self._service._lower , self._service._upper), (160, 800))
+ def test_ks_reserve_mb(self):
self._check_ks_input("""
|
it's your turn, pls close this pr and commit a new one. |
|
I've merged this PR and created a separate commit 8f61cc1 ("test: patch where an object is looked up") to fix the mocking issue. Thank you for your contribution to this project! |
we need to check reserve-mb number is in range of the lower and upper when we use ks command.
we know the range with tui, and we also can see the range with the gui